-
Notifications
You must be signed in to change notification settings - Fork 444
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Contributing Intel Tofino compiler backend to p4c #4964
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exciting!
I would consider using something like https://github.com/p4lang/p4c/blob/main/backends/p4tools/cmake/common.cmake#L16 to fix the Z3 issues. At least it has worked well in the tools back end. We can put that script into top-level cmake folder as Z3.cmake.
CMakeLists.txt
Outdated
@@ -35,6 +35,7 @@ set (CMAKE_USE_RELATIVE_PATHS 1) | |||
|
|||
OPTION (ENABLE_DOCS "Build the documentation" OFF) | |||
OPTION (ENABLE_GTESTS "Enable building and running GTest unit tests" ON) | |||
OPTION (ENABLE_TOFINO "Build the TOFINO backend (required for the full test suite)" ON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would disable this by default and only build Tofino on the Ubuntu 22.04 Github actions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing is that this backend is really... large :) So not all breaking changes might be quite non-trivial :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing is that this backend is really... large :) So not all breaking changes might be quite non-trivial :)
This might actually be a good thing. bf-p4c pushes the limits of what you can do as a back end. So if we introduce breaking changes it can act as a canary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it has both pros and cons. I am talking mostly about the amount of possible changes required if we'd introduce, say, some IR change or something like this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial round of comments.
@@ -374,6 +375,7 @@ include_directories ( | |||
${P4C_SOURCE_DIR} | |||
${P4C_BINARY_DIR} | |||
${P4C_SOURCE_DIR}/extensions | |||
${P4C_SOURCE_DIR}/backends/tofino ## FIXME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am guessing this refers to fixing the include paths?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this should not be needed -- but I guess just removing it currently breaks things? If it is needed, it should probably be in backends/tofino/CMakeLists.txt, but I'm unsure how cmake nesting works in general.
tools/driver/CMakeLists.txt
Outdated
add_test(NAME driver_inputs_test_2 COMMAND ${P4C_SOURCE_DIR}/tools/driver/test_scripts/driver_inputs_test_2) | ||
add_test(NAME driver_inputs_test_3 COMMAND ${P4C_SOURCE_DIR}/tools/driver/test_scripts/driver_inputs_test_3) | ||
add_test(NAME driver_inputs_test_4 COMMAND ${P4C_SOURCE_DIR}/tools/driver/test_scripts/driver_inputs_test_4) | ||
### Disabled to unblock CI for Tofino backend. Will reenable after I figure out the reason of the test failure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the PR is blocked on this? What are the failures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear, probably because the script assumes the top level project name is p4c, instead of bf-p4c? I cannot reproduce the error locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hold on we should fix this first before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give it a try to reproduce the error in your local environment/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you remove the comments, what is the CI error? Will try to build bf-p4c soon, but not sure when.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the bf-p4c repo, this is the error:
FATAL ERROR: could not find the P4 compiler driver. Searched in the following locations...
///>>>./p4c<<<///
///>>>../p4c<<<///
///>>>../../p4c<<<///
///>>>../p4c/p4c<<<///
///>>>../../p4c/p4c<<<///
///>>>/home/runner/work/bf-p4c/bf-p4c<<<///
///>>>/home/runner/work/bf-p4c/bf-p4c/p4c<<<///
///>>>/home/runner/work/bf-p4c/bf-p4c/p4c/p4c<<<///
///>>>/home/runner/work/bf-p4c<<<///
///>>>/home/runner/work/bf-p4c/p4c<<<///
///>>>/home/runner/work/bf-p4c/p4c/p4c<<<///
///>>>/home/runner/work/_temp//p4c<<<///
///>>>/home/runner/work/_temp//p4c/p4c<<<///
Unable to find the driver of the P4 compiler. Aborting the test ''/home/runner/work/bf-p4c/bf-p4c/tools/driver/test_scripts/driver_inputs_test_3___three_bad_pathnames.bash'' with a non-zero exit code. This test failed.
enable_testing() | ||
endif() | ||
|
||
set(BOOST_MIN_VERSION "1.58.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much boost is left in the compiler? We got rid of a lot of it in the open-source compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are still some, phv depends on boost::graph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am seeing use of:
- boost/range / boost::irange
- boost/algorithms/string (use abseil instead)
- boost/iostreams
- boost/none / optional (certainly should be removed in favour of standard ones)
- boost/uuid
- boost/functional (mostly boost::hash, replace with Util::Hash)
- boost/iterator
- boost/format
- boost/property_map
- boost/bind (replace with lambda's and C++17 features)
- boost/operators
- boost/make_unique
Plus some code to support legacy pre-C++17 things here and there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, most of those we should be able to clean up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#4663 could actually help with identifying extra boost stuff, the build should break for includes that we did not declare. Will rebase after this PR is merged.
Given the size of the backend – who is supposed to support it after the merge? I am worrying that any project-wide changes will become very time-consuming given the size of the backend. Maybe it should live in a separate repo and rebase from time to time to p4c mainline? |
@grg @hanw Have you had any discussions about this topic? I thought that perhaps Glen had mentioned an approach like this at one point, and while there is a risk of the Tofino back end being left behind for long stretches of time until someone catches it up, it does have some advantages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is too large to properly review, but as a self contained backend, the main concern is how well it integrates with the rest of the code.
My recollection is that including this backend slows down the build-time A LOT, so that may be an issue.
I have a little bit of concern about the license, as it appears to not be Apache licensed, but IANAL, so I can only suggest getting the opinion of someone who is a copyright lawyer.
@@ -374,6 +375,7 @@ include_directories ( | |||
${P4C_SOURCE_DIR} | |||
${P4C_BINARY_DIR} | |||
${P4C_SOURCE_DIR}/extensions | |||
${P4C_SOURCE_DIR}/backends/tofino ## FIXME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this should not be needed -- but I guess just removing it currently breaks things? If it is needed, it should probably be in backends/tofino/CMakeLists.txt, but I'm unsure how cmake nesting works in general.
COMMENT "Building ctags") | ||
|
||
add_custom_target(cpplint-all | ||
DEPENDS cpplint-asm cpplint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since bfasm isn't present in this code, there probably should not be dependencies on it here?
set print object | ||
set unwindonsignal on | ||
set unwind-on-terminating-exception on | ||
set python print-stack full |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be merged with the top-level .gdbinit file -- it shares a lot in common with that in any case
We may consider either:
|
Seems like my comment ended up in the ether somewhere, reposting:
We discussed this and the best option is to have the Tofino compiler as part of the open-source P4C to avoid bit-rotting. However, in order to not encumber the rest of the compiler too much this back end will be part of a separate PR that is optional. There is an advantage to this approach: the front end of the compiler has largely been designed for the Tofino back end. Now that both are integrated with each other we can start to be more aggressive in refactorings or optimizations because we immediately see the effects on the Tofino back end. I expect things to improve quickly. We can also see how this works out in practice over the next couple of weeks.
We will probably go with option 2, but have it run similar to the Fedora tests (on every commit, but not required for merging). |
Well, my feeling is that it puts very huge burden on not-so-large community and effectively prevents many project-wide changes. Certainly, it could be optional and not required for merging. So, does this mean, it will just become broken at any breaking change, is this the intention? Bit-rotting is unavoidable if there is noone to support it.
Let me drop a small example. Let's say I need to do some IR-related refactoring that requires changes everywhere. If I need to spend, say, 8 hours to do this for frontend + midend + existing backends and spend another, say, 40 hours just for Tofino, will I be willing to make such change? Highly unlikely. Or I will just leave Tofino on the side and expect someone else to make this port? |
Intel will have engineers to maintain the code. The fact that it's open-sourced makes it easier for people that are not at Intel to contribute. |
backends/tofino/LICENSE
Outdated
@@ -0,0 +1,10 @@ | |||
Copyright 2013-2024 Intel Corporation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's outdated, will fix.
Thanks for the clarification. So, am I reading the policy correctly?
|
And if this is true, it would be good to always publish a short list of commit SHAs somewhere, for which the Tofino back end builds, and has been well-tested. |
I had to port breaking changes to bf-p4c in the past. I also currently have a couple of PRs open that will require fixing once the Tofino compiler is merged. Overall, the code base seems daunting but a lot of the code is fairly self-contained. I estimate things will only improve once we start to clean up the back end and move some of its parts into the core. There is a lot of redundancy that we can get rid of. I'd say we evaluate how things are going for a couple of weeks and if we notice that the burden is to high we can still factor out the compiler. There is very little practical difference between a back end and an extension so it should not be a big change. |
This aligns with my understanding. |
I agree with this approach. |
.github/workflows/ci-test-debian.yml
Outdated
@@ -21,6 +21,7 @@ jobs: | |||
CTEST_PARALLEL_LEVEL: 4 | |||
IMAGE_TYPE: test | |||
BUILD_GENERATOR: Ninja | |||
ENABLE_TOFINO: ON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would create a new job test-ubuntu22-tofino` which is a copy of this job and enable Tofino there. We can also disable all other back ends to save compilation time.
tools/driver/CMakeLists.txt
Outdated
add_test(NAME driver_inputs_test_2 COMMAND ${P4C_SOURCE_DIR}/tools/driver/test_scripts/driver_inputs_test_2) | ||
add_test(NAME driver_inputs_test_3 COMMAND ${P4C_SOURCE_DIR}/tools/driver/test_scripts/driver_inputs_test_3) | ||
add_test(NAME driver_inputs_test_4 COMMAND ${P4C_SOURCE_DIR}/tools/driver/test_scripts/driver_inputs_test_4) | ||
### Disabled to unblock CI for Tofino backend. Will reenable after I figure out the reason of the test failure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hold on we should fix this first before merging.
Co-authored-by: Glen Gibb <[email protected]> Co-authored-by: Prathima Kotikalapudi <[email protected]> Co-authored-by: Han Wang <[email protected]> Signed-off-by: Han Wang <[email protected]>
Co-authored-by: Glen Gibb <[email protected]> Co-authored-by: Prathima Kotikalapudi <[email protected]> Co-authored-by: Han Wang <[email protected]> Signed-off-by: Han Wang <[email protected]>
Co-authored-by: Glen Gibb <[email protected]> Co-authored-by: Prathima Kotikalapudi <[email protected]> Co-authored-by: Han Wang <[email protected]> Signed-off-by: Han Wang <[email protected]>
Co-authored-by: Glen Gibb <[email protected]> Co-authored-by: Prathima Kotikalapudi <[email protected]> Co-authored-by: Han Wang <[email protected]> Signed-off-by: Han Wang <[email protected]>
Co-authored-by: Glen Gibb <[email protected]> Co-authored-by: Prathima Kotikalapudi <[email protected]> Co-authored-by: Han Wang <[email protected]> Signed-off-by: Han Wang <[email protected]>
- fix ci-build.sh - add pyinstaller to requirement.txt - link ${P4C_LIB_DEPS} to bf_gtest_support target - link ${P4C_LIB_DEPS} to tofinobackend target Co-authored-by: Glen Gibb <[email protected]> Co-authored-by: Prathima Kotikalapudi <[email protected]> Co-authored-by: Han Wang <[email protected]> Signed-off-by: Han Wang <[email protected]>
- clang-format - black format - isort errors - ifdefs name fixes Co-authored-by: Glen Gibb <[email protected]> Co-authored-by: Prathima Kotikalapudi <[email protected]> Co-authored-by: Han Wang <[email protected]> Signed-off-by: Han Wang <[email protected]>
Co-authored-by: Glen Gibb <[email protected]> Co-authored-by: Prathima Kotikalapudi <[email protected]> Co-authored-by: Han Wang <[email protected]> Signed-off-by: Han Wang <[email protected]>
Co-authored-by: Glen Gibb <[email protected]> Co-authored-by: Prathima Kotikalapudi <[email protected]> Co-authored-by: Han Wang <[email protected]> Signed-off-by: Han Wang <[email protected]>
Co-authored-by: Glen Gibb <[email protected]> Co-authored-by: Prathima Kotikalapudi <[email protected]> Co-authored-by: Han Wang <[email protected]> Signed-off-by: Han Wang <[email protected]>
- remove duplicated bmv2/psa.p4 and dpdk/psa.p4 - remove calling bfas in p4c-driver - disable a few driver tests Co-authored-by: Glen Gibb <[email protected]> Co-authored-by: Prathima Kotikalapudi <[email protected]> Co-authored-by: Han Wang <[email protected]> Signed-off-by: Han Wang <[email protected]>
Co-authored-by: Glen Gibb <[email protected]> Co-authored-by: Prathima Kotikalapudi <[email protected]> Co-authored-by: Han Wang <[email protected]> Signed-off-by: Han Wang <[email protected]>
Co-authored-by: Glen Gibb <[email protected]> Co-authored-by: Prathima Kotikalapudi <[email protected]> Co-authored-by: Han Wang <[email protected]> Signed-off-by: Han Wang <[email protected]>
Co-authored-by: Glen Gibb <[email protected]> Co-authored-by: Prathima Kotikalapudi <[email protected]> Co-authored-by: Han Wang <[email protected]> Signed-off-by: Han Wang <[email protected]>
Co-authored-by: Glen Gibb <[email protected]> Co-authored-by: Prathima Kotikalapudi <[email protected]> Co-authored-by: Han Wang <[email protected]> Signed-off-by: Han Wang <[email protected]>
This PR is large enough. Let's visit boost libraries in a follow-up PR. I also need to add a regression suite before further editing the source code. |
Co-authored-by: Glen Gibb <[email protected]> Co-authored-by: Prathima Kotikalapudi <[email protected]> Co-authored-by: Han Wang <[email protected]> Signed-off-by: Han Wang <[email protected]>
Let's wait until Friday in case someone else has concerns. If there are no further comments at that point we can merge this PR then. |
@hanw, any chance the Tofino Testgen target will be also contributed? |
This pull request introduces the compiler backend for Intel Tofino 1 and Tofino 2 programmable switch ASICs, licensed under the Apache License 2.0. Due to the large number of files, this PR has been divided into a series of commits, organized primarily by directory structure.
To build the backend, use the
-DENABLE_TOFINO=ON
flag during bootstrapping. This flag is disabled by default.Please note that this PR contains only the source code for the Tofino backend and does not include additional regression tests. A separate PR will be submitted to provide a set of PTF and STF tests for this backend.
A big thank you to all the team members who contributed to this compiler -- you know who you are!
-- Glen, Prathima, Han